Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Temp: Proposal for ComposedWebView #845

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ldoppea
Copy link
Member

@Ldoppea Ldoppea commented Jun 22, 2023

This PR is a proposal for a refactoring on our WebView management.

What I'm waiting from you are some feedback on the proposal, if you think is a good idea or not. I'm currently stuck on the implementation and I don't want to invest more time until I'm sure it is the good direction.

Problem

Today our codebase structure is the following:

HomeView
└ CozyProxyWebView
   └ CozyWebView
      └ ReloadInterceptorWebView
         └ SupervisedWebView

Each WebView component has a hard-coded reference to its WebView child (i.e CozyWebView defines that its child is a ReloadInterceptorWebView)

This makes difficult to add a layer to the WebView's tree, or to invert some of them in the hierarchy.

Imagine that we want to add a ReloadableWebView that embed ALL of our webviews and allows to unmount/remount everything when we want a reload (so we enforce the proxywebview to generate a new HTML)

To make this we would have to edit HomeView to use a ReloadableWebView instead of CozyProxyWebView, to hard code ReloadableWebView with CozyProxyWebView as child, and then we should remember to search for all other CozyProxyWebView references in the code and replace CozyProxyWebView by ReloadableWebView like we did in HomeView

Here would be the new codebase structure

HomeView
└ ReloadableWebView
  └ CozyProxyWebView
     └ CozyWebView
        └ ReloadInterceptorWebView
           └ SupervisedWebView

So my question is How can we make this more flexible?

Proposal

My proposal is to create a ComposedWebView that would allow developers to "compose" their WebView like they want.

The API would be like this:

return (
  <ComposedWebView
      {...props}
      childrenTypes={[
        CozyProxyWebView,
        CozyWebView,
        ReloadInterceptorWebView,
        SupervisedWebView
      ]}
    />
)

Which would be translated on runtime as the following

CozyProxyWebView
└ CozyWebView
   └ ReloadInterceptorWebView
      └ SupervisedWebView

This would allow to easily add/remove parts. If we want to remove the ReloadInterceptorWebView from the generated tree then we can do:

return (
  <ComposedWebView
      {...props}
      childrenTypes={[
        CozyProxyWebView,
        CozyWebView,
        //ReloadInterceptorWebView,
        SupervisedWebView
      ]}
    />
)

If we want to add a ReloadableWebView on the top level then we can do:

return (
  <ComposedWebView
      {...props}
      childrenTypes={[
        ReloadableWebView,
        CozyProxyWebView,
        CozyWebView,
        ReloadInterceptorWebView,
        SupervisedWebView
      ]}
    />
)

Work in progress

This PR is a draft implementation for this concept.

src/components/webviews/ComposedWebView.js contains the logic that translates the flat declaration into a nested WebView tree on runtime.

This would have some repercussions on the way we create WebViews:

  • WebView components must be wrapped in a forwardRef
  • WebView components must take a ChildWebview prop defaulted to WebView and use it as their child
  • All WebView methods like onShouldStartLoadWithRequest should consider they can have a parent and call it if exists
    • Example:
<ChildWebView
  onShouldStartLoadWithRequest={(initialRequest) => {
    if (/* some condition */) {
      // do stuff
      return false
    }

    if (props.onShouldStartLoadWithRequest) {
      return props.onShouldStartLoadWithRequest(initialRequest)
    }  else {
      return true
    }
  }}
/>

⚠️ Current implementation is not complete and has some issues:

  • ReloadInterceptorWebView gets nodeHandle errors
    • but if we remove it from the tree then everything seems to work
  • CozyAppScreen will do an infinite loop
    • however HomeView seems to work correctly

@Crash--
Copy link
Contributor

Crash-- commented Jun 22, 2023

Maybe linked to #411 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants